[WIP] feat: add share button for flow links #3483
Conversation
|
Removed vultr server and associated DNS entries |
RODO94
force-pushed
the
rory/shareable-flow-url
branch
from
3fc6375 to
4317017
Compare
RODO94
force-pushed
the
rory/shareable-flow-url
branch
from
3e9295c to
68760d9
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
Using Buckinghamshire as a team is good for testing the Subdomain flow, I updated the Hasura with a close to real dummy data. |
There was a problem hiding this comment.
Took a quick first pass at this! Looking like a really solid start, but finding some of the styling and conditional syntax used throughout to be a bit confusing - let's talk through any initial feedback points / questions if they aren't clear!
Best tip I can offer is "do less" / try to reference more from our already existing MUI global theme ! Personally I think our MUI theme & existing style patterns should be your source of truth here, and Figma as a visual guide only (we almost never use Figmas to pixel exactness).
| @@ -24,6 +24,8 @@ const Flow = ({ breadcrumbs = [] }: any) => { | |||
| href: `${window.location.pathname.split(id)[0]}${id}`, | |||
| })); | |||
|
|
|||
| console.log(useStore.getState().flowStatus); | |||
There was a problem hiding this comment.
Oops lingering console.log, please remove!
| backgroundColor: | ||
| isOutsideEditor || environment === "standalone" | ||
| ? theme.primaryColour | ||
| : "#2c2c2c", |
There was a problem hiding this comment.
I'm not sure I'm following the introduction of isOutsideEditor and why this new backgroundColor condition is necessary?
state.previewEnvironment does semi-confusingly return "editor" rather than "standalone" for /draft & /preview routes - but I think we'd want to update this directly rather than introduce a new variable?
Also, broadly, how does changing the header color related to sharing links?
|
|
||
| const PaddedText = styled(Typography)(({ theme }) => ({ | ||
| paddingLeft: "31px", | ||
| })); |
There was a problem hiding this comment.
nit: in my opinion, defining a styled component is overkill if there's just a few configs - I personally think the inline sx prop is cleaner in these cases, eg:
<Typography sx={{ paddingLeft: "31px" }}>
{...}
</Typograhy>
| return ( | ||
| <Tooltip title={copyMessage}> | ||
| <Button | ||
| component={"button"} |
There was a problem hiding this comment.
nit: is component={"button"} redundant here?
| : props.isFlowPublished && flowStatus !== "online" | ||
| ? publishedOfflineDescription | ||
| : publishedOnlineDescription | ||
| } |
There was a problem hiding this comment.
nit: anytime a ternary is more than a single condition, I think it's helpful for readability to extract into an external helper function that can use more verbose if/else or switch syntax?
Eg description={getLinkDescription(isPublished, status)}
This also has benefit of becoming something we can test indepedently!
|
|
||
| const SecondaryButton = styled(IconButton)(({ theme }) => ({ | ||
| color: theme.palette.common.black, | ||
| })); |
There was a problem hiding this comment.
nit: MUI's IconButton accepts a color prop which we can use to reference our standard "secondary" color per the global theme - would that be more appropriate here?
Personally find it quite confusing to be defining bespoke "secondary" styles and straying from global theme.
| padding={"0px 20px"} | ||
| gap={"8px"} | ||
| > | ||
| <SecondaryButton color="primary"> |
There was a problem hiding this comment.
I find <SecondaryButton color="primary"> very semantically confusing!! See related comment above.
| flexDirection: "row", | ||
| gap: "8px", | ||
| borderRadius: "5px", | ||
| padding: "8px", |
There was a problem hiding this comment.
nit: Feels a bit surprising & brittle to me to see properties like display, flexDirection, and gap defined directly on a Button - I think I'd expect to see a wrapper Box/div with these properties that then accepts buttons as children ??
| <ActivePublishedLink title="PlanX link" link={props.link} /> | ||
| ) : ( | ||
| <InactivePublishedLink title="PlanX link" message={props.link} /> | ||
| )} |
There was a problem hiding this comment.
I find this ternary very difficult to read, and similar to another comment would much prefer more verbose if/else or switch syntax here to describe the expected conditional rendering rules.
Also a great example where tests would help describe expected behavior here !
| isPublished={props.isPublished} | ||
| /> | ||
| ) : ( | ||
| <Link pl={"31px"} href={props.link}> |
There was a problem hiding this comment.
nit: noticing lots of places throughout these file changes where 31px is hard-coded - it'd be much more ideal to try to use an existing global theme.spacing value here I think ?
Closed BranchBranch has been closed as the design has progressed away from this iteration. It may be picked up later in time, so the branch will stay, but the PR will remain closed. A new branch and PR will be made for the new iteration. |
What does this PR do?
This PR comes from the Trello ticker: https://trello.com/c/k8cQojk7/2958-share-service-link-in-editor
The intention is to add a share link or copy link button to the FlowEditor page for users to easily see and share the right links for their flows.
From the Trello ticket:
I worked through the Figma designs given here: Figma mock-ups
There has been a range of components added, created, and altered. From the new styled buttons,
CopyButtonandToggleButton.The reuse of the
ImageWrappercomponent used previously in Design Settings but customised to new sizes, I thought keeping the name consistent would point at similar use but different settings, since neither are exported from their origin.A
LinkComponentwas made (naming may change to be more explicit) to harmonise styling across all the different links in theDialogbox, this includes the ability to add either an icon or image or neither to the Title of a link.The
ToggleButtoncomponent was used to move the sidebar toggle from the top header to the Sidebar, in line with the other buttons for publishing and sharing links. This has been positionedabsolutewith a positionrelativeset around the box containing the buttons so it can hang over the left edge of the siderbar.Feedback requests:
Feel free to give me feedback on anything, but these are the particular areas I found many options in.
<LinkComponentTo Do's
refreshandconsole